feat(tcpreuse): add options for sharing TCP listeners amongst TCP, WS and WSS transports #2984

aschmahmann · 2024-09-27T17:02:05Z

@MarcoPolo @sukunrt I took a stab at this as a result of looking at ipfs/kubo#10521. I also pulled from @Jorropo's work on #2737.

Initial testing seems to indicate things work ok, but this definitely needs more tests, API cleanup + eyes before merging.

Side note: Given that we're sharing the underlying TCP listener now I wonder if we basically have to tackle #1435 as well.

p2p/transport/tcp/tcp.go

aschmahmann · 2024-09-27T17:07:51Z

p2p/transport/tcpreuse/demultiplex.go

+const (
+	Unknown DemultiplexedConnType = iota
+	MultistreamSelect
+	HTTP
+	TLS
+)


This setup is hardcoded and doesn't allow others to add new protocols to demultiplex from the same listener without making a PR. This seems ok given we have to be careful about making sure we don't get magic byte collisions between different protocols, but if we want to set this up as a registry object instead that seems fine too.

agreed. Thought needs to put into extending this. Compared to that, adding a new enum here is easy.

p2p/transport/tcpreuse/demultiplex.go

aschmahmann · 2024-09-27T17:18:48Z

p2p/transport/tcpreuse/demultiplex.go

+
+func IsTLS(s Sample) bool {
+	switch string(s[:]) {
+	case "\x16\x03\x01", "\x16\x03\x02", "\x16\x03\x03", "\x16\x03\x04":


I'm pretty sure from reading https://datatracker.ietf.org/doc/html/rfc8446#section-4.1.2 it seems like \x16\x03\x04 is never expected in the client hello and so should be here. (fyi @Jorropo since I took this from magiselect)

Do you mean so should not be here?

p2p/transport/tcpreuse/demultiplex.go

p2p/transport/tcpreuse/listener.go

aschmahmann · 2024-09-27T17:27:17Z

p2p/transport/tcp/tcp.go

@@ -122,6 +133,9 @@ type TcpTransport struct {
 	disableReuseport bool // Explicitly disable reuseport.
 	enableMetrics    bool

+	// share and demultiplex TCP listeners across multiple transports
+	sharedTcp *tcpreuse.ConnMgr


Suggestions on what to call this feature/option? I grabbed the ConnMgr name because it's used in the quicreuse package, but it doesn't seem quite right and tcpreuse seems likely to be confused with reuseport.

Not sure. I also don't like the connmgr name in quicreuse. But I don't have a better name that isn't also confusing. At least this is consistently confusing (?)

aschmahmann · 2024-09-27T17:29:41Z

p2p/transport/tcpreuse/listener.go

+}
+
+func (m *multiplexedListener) Run() error {
+	const numWorkers = 16


We can have preset workers, options, etc. but I wonder if the resource manager might also help with the job here. My understanding is that it's used starting the multistream level because it won't play nicely with just a net.Conn, maybe this means we can move the resource management lower in the stack.

I think this was refactored to use a channel semaphore with N=64. That should be good enough for most (all?) use cases.

This thing should be very fast we are reading the first 3 bytes of the first packet.

aschmahmann · 2024-09-27T17:36:56Z

p2p/transport/tcpreuse/reuseport.go

+// EnvReuseportVal stores the value of envReuseport. defaults to true.
+var EnvReuseportVal = true


This doesn't feel good to export as it's only used for tests and is the result of me moving things to avoid dependency cycles (and separate concerns).

Any suggestions on where this reuseport logic lives? Could potentially move the tests or have enough options to thread the testing logic through

There's a refactor to be done here, but maybe we do that in a separate PR? This one is already quite big

sukunrt · 2024-10-01T14:53:08Z

Overall it looks quite good.

The only thing major thing that we must fix before merging is this:

// TODO: if/how do we want to handle stalled connections and stop them from clogging up the pipeline?
// Drop connection because the buffer is full

We need a way to limit the number of connections being upgraded per IP/subnet. Otherwise, a single node can make the target node unresponsive.

We don't have the endpoint multiaddr at this point. This makes it difficult to use the resource manager as is. It's not too bad though, The only useful part of endpoint is (IP,PORT,TCP/UDP) tuple.

	OpenConnection(dir Direction, usefd bool, endpoint multiaddr.Multiaddr) (ConnManagementScope, error)

We can just use the rcmgr as is. We have the IP and Port of the remote, that's all that's needed by the resource manager any way.
We can implement the ConnLimiter separately for this tcp reuse block.

I think 1 is fine. AllowList anyway works with a IP / Subnet. We can make this clear in the documentation for Resource Manager.

sukunrt · 2024-10-07T10:56:21Z

p2p/transport/tcp/tcp.go

+func NewTCPTransport(upgrader transport.Upgrader, rcmgr network.ResourceManager, sharedTCP *tcpreuse.ConnMgr, opts ...Option) (*TcpTransport, error) {
 	if rcmgr == nil {


This is a breaking change.
It simplifies the top level basic host api a lot. You can do:

libp2p.New(Transport(tcp.NewTransport), Transport(websocket.New), ShareTCPListener())

And not need to specify the rcmgr, or the connection gater.

I'm confused how to provide this API, enabling sharing by just specifying ShareTCPListener, and pass in the sharedTCP objects as one of the optional arguments TCP Transport. If we can pass in the sharedTCP object as an optional argument that will prevent breaking this.

The simplest alternative implementation wise, without any breaking change, is an API like:

tcmgr := tcpreuse.NewConnMgr(false, gater, rcmgr) libp2p.New( Transport(tcp.NewTransport, tcp.WithSharedTCP(tcmgr)), Transport(websocket.New, websocket.WithSharedTCP(tcmgr)), )

p2p/transport/tcpreuse/listener.go

sukunrt · 2024-10-07T10:59:08Z

p2p/transport/tcpreuse/listener.go

+		// Gate and resource limit the connection here.
+		// If done after sampling the connection, we'll be vulnerable to DOS attacks by a single peer
+		// which clogs up our entire connection queue.
+		// This duplicates the responsibility of gating and resource limiting between here and the upgrader. The
+		// alternative without duplication requires moving the process of upgrading the connection here, which forces
+		// us to establish the websocket connection here. That is more duplication, or a significant breaking change.
+		//
+		// Bugs around multiple calls to OpenConnection or InterceptAccept are prevented by the transport
+		// integration tests.


This is the smallest change I could think of which provided this DoS by a single node prevention.
It's also nicely guarded by transport integration tests, so this feels okay to me.

p2p/transport/tcpreuse/listener_test.go

sukunrt · 2024-10-07T11:02:08Z

p2p/transport/websocket/listener.go

-		mnc, err := manet.WrapNetConn(c)
-		if err != nil {
-			c.Close()
-			return nil, err
-		}
-		return mnc, nil
+		return c, nil


Wrapping it by manet shadows the Scope method which we need for the upgrader. Any regression here is prevented by transport integration tests.

sukunrt · 2024-10-07T11:03:16Z

options.go

+// ShareTCPListener shares the same listen address between TCP and Websocket transports.
+func ShareTCPListener() Option {
+	return func(cfg *Config) error {
+		cfg.ShareTCPListener = true
+		return nil
+	}
+}


This API has the nice property of keeping all current code same and enabling sharing by just specifying

libp2p.New( ... ShareTCPListener() )

As libp2p.New user I will wonder why I need to explicitly pass this Option to enable sharing for TCP ports, but don't have to do anything extra for UDP ones.

No chance of making this implicit? Or at least explain in godoc comment why this needs to be explicit?

The main reason is that I want to get some confidence here before defaulting this behavior to on. In a future release it'll be the default and you wouldn't need this.

Aside: UDP transports don't currently share the port if you use port 0 across them (this does). That's also something that I'd like to change in a future release.

I'll update the comment.

…, and WSS transports

MarcoPolo

I think this mostly looks good. Just a couple things I'll clean up in the next pass

p2p/transport/tcp/tcp.go

p2p/transport/websocket/websocket.go

p2p/transport/tcpreuse/listener.go

MarcoPolo · 2024-10-29T03:10:50Z

p2p/transport/tcpreuse/demultiplex.go

+const (
+	Unknown DemultiplexedConnType = iota
+	MultistreamSelect
+	HTTP
+	TLS
+)


agreed. Thought needs to put into extending this. Compared to that, adding a new enum here is easy.

p2p/transport/tcpreuse/demultiplex.go

libp2p_test.go

p2p/net/upgrader/listener.go

p2p/transport/tcp/metrics.go

p2p/transport/tcpreuse/demultiplex.go

p2p/transport/websocket/conn.go

config/config.go

…ntype

aschmahmann commented Sep 27, 2024

View reviewed changes

aschmahmann force-pushed the feat/shared-tcp branch from b539b9e to b6f1fb0 Compare September 27, 2024 17:31

aschmahmann commented Sep 27, 2024

View reviewed changes

sukunrt force-pushed the feat/shared-tcp branch from 6f21a3f to 60cef4d Compare October 5, 2024 18:27

sukunrt reviewed Oct 7, 2024

View reviewed changes

sukunrt requested a review from MarcoPolo October 7, 2024 11:03

sukunrt marked this pull request as ready for review October 7, 2024 11:04

MarcoPolo mentioned this pull request Oct 9, 2024

v0.37 #2901

Closed

30 tasks

MarcoPolo mentioned this pull request Oct 28, 2024

v0.38 #3005

Open

20 tasks

This was referenced Oct 29, 2024

feat(AutoTLS): opt-in WSS certs from p2p-forge at libp2p.direct ipfs/kubo#10521

Merged

Enabling AutoTLS feature by default ipfs/kubo#10560

Open

MarcoPolo self-assigned this Oct 29, 2024

aschmahmann and others added 12 commits October 30, 2024 10:56

feat(tcpreuse): add options for sharing TCP listeners amongst TCP, WS…

6281130

…, and WSS transports

add tests for listener

2ae20f4

create conn scope early to prevent DoS attacks

05ee445

add fx option, move rcmgr and upgrader to sharedtcp

3457a7b

make fewer concurrent connections in test, it breaks mac and windows

ac8b478

add some comments

6d5fa53

Add OS specific sampledconn

0115a62

Return net.Conn unwrapped if possible

cdbf16c

feat(tcpreuse): add Windows sampledconn

7f4f875

Add test for metrics+unix

00c9403

tcp transport: Parameterize metrics collector in TCP

2c053d8

simplify demultiplex a bit

7e34d05

MarcoPolo force-pushed the feat/shared-tcp branch from c3ff407 to 7e34d05 Compare October 30, 2024 18:10

MarcoPolo added 3 commits October 30, 2024 12:20

transport-testsuite(old): Parameterize subtests

e465b29

tcp-transport: selectively run only 1conn tests

677222a

sampledconn: update tests to be manet aware

8c334af

MarcoPolo added 3 commits October 30, 2024 13:11

Remove unused interface

b686b80

sampledconn: Add test case back in

a28a244

Comment nits

a39490f

MarcoPolo reviewed Oct 30, 2024

View reviewed changes

aschmahmann mentioned this pull request Oct 30, 2024

feat(libp2p): enable shared TCP listeners ipfs/kubo#10565

Open

aschmahmann commented Oct 30, 2024

View reviewed changes

config/config.go Show resolved Hide resolved

MarcoPolo added 2 commits October 30, 2024 20:03

tcpreuse: flip reuseport bool

76f560b

tcpreuse: return an error on multiple listeners for the same addr+con…

c314a2a

…ntype

MarcoPolo force-pushed the feat/shared-tcp branch from cadd881 to c314a2a Compare October 31, 2024 03:03

MarcoPolo added 9 commits October 30, 2024 20:10

websocket: return consistent error

56dabd4

Remove todo

9ebfc89

tcp: revert parameterize metrics collector

a722a7c

typo

85be1f5

PR review

18af914

typo

f9f39b7

add timeout

9ca4ae7

pr nit

3d26583

remove unused option

3487369

MarcoPolo force-pushed the feat/shared-tcp branch from 8bbc8a7 to 3487369 Compare October 31, 2024 03:31

MarcoPolo added 2 commits October 30, 2024 20:51

Expand comment

74e9c05

remove 0x160304 magic byte match

fd53643

MarcoPolo approved these changes Oct 31, 2024

View reviewed changes

Fix test to handle existing listener error

ccd1609

MarcoPolo merged commit 5a47a90 into master Nov 4, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tcpreuse): add options for sharing TCP listeners amongst TCP, WS and WSS transports #2984

feat(tcpreuse): add options for sharing TCP listeners amongst TCP, WS and WSS transports #2984

aschmahmann commented Sep 27, 2024

aschmahmann Sep 27, 2024

MarcoPolo Oct 29, 2024

aschmahmann Sep 27, 2024

MarcoPolo Oct 31, 2024

aschmahmann Oct 31, 2024

aschmahmann Sep 27, 2024

MarcoPolo Oct 31, 2024

aschmahmann Sep 27, 2024

MarcoPolo Oct 31, 2024

aschmahmann Sep 27, 2024

MarcoPolo Oct 31, 2024

sukunrt commented Oct 1, 2024 •

edited

Loading

sukunrt Oct 7, 2024 •

edited

Loading

sukunrt Oct 7, 2024

sukunrt Oct 7, 2024 •

edited

Loading

sukunrt Oct 7, 2024 •

edited

Loading

lidel Oct 30, 2024

MarcoPolo Oct 31, 2024

MarcoPolo left a comment

MarcoPolo Oct 29, 2024

		// EnvReuseportVal stores the value of envReuseport. defaults to true.
		var EnvReuseportVal = true

		func NewTCPTransport(upgrader transport.Upgrader, rcmgr network.ResourceManager, sharedTCP tcpreuse.ConnMgr, opts ...Option) (TcpTransport, error) {
		if rcmgr == nil {

feat(tcpreuse): add options for sharing TCP listeners amongst TCP, WS and WSS transports #2984

feat(tcpreuse): add options for sharing TCP listeners amongst TCP, WS and WSS transports #2984

Conversation

aschmahmann commented Sep 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sukunrt commented Oct 1, 2024 • edited Loading

sukunrt Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sukunrt Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

sukunrt Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoPolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sukunrt commented Oct 1, 2024 •

edited

Loading

sukunrt Oct 7, 2024 •

edited

Loading

sukunrt Oct 7, 2024 •

edited

Loading

sukunrt Oct 7, 2024 •

edited

Loading